1934877 - Memory leak in tls13_MaybeGreaseEch

Maurice Dauer [:mdauer]

Reporter

Description

•

6 months ago

Attached file clusterfuzz-testcase-tls-client-5766569436708864 — Details

OSS-Fuzz: https://oss-fuzz.com/testcase-detail/5766569436708864

Details

My uneducated guess would be that we reach this line twice, overwriting ss->ssl3.hs.greaseEchBuf, without having freed the previously allocated greaseBuf.

Reproduction

Download the attached testcase
Build NSS with ./build.sh -c --fuzz=tls
Run /path/to/dist/Debug/bin/nssfuzz-tls-client /path/to/testcase

Stack trace

Direct leak of 1064 byte(s) in 1 object(s) allocated from:
	    #0 0x5a114c971d6c in __interceptor_realloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:82:3
	    #1 0x5a114cadafd9 in PORT_Realloc_Util nss/lib/util/secport.c:101:14
	    #2 0x5a114cabee7c in sslBuffer_Grow nss/lib/ssl/sslencode.c:50:39
	    #3 0x5a114cabf6f2 in sslBuffer_AppendVariable nss/lib/ssl/sslencode.c:117:9
	    #4 0x5a114ca2ef18 in tls13_MaybeGreaseEch nss/lib/ssl/tls13ech.c:2258:10
	    #5 0x5a114ca6cb5a in ssl3_SendClientHello nss/lib/ssl/ssl3con.c:5620:18
	    #6 0x5a114ca9b833 in ssl3_HandleHelloRequest nss/lib/ssl/ssl3con.c:5787:10
	    #7 0x5a114ca84635 in ssl3_HandlePostHelloHandshakeMessage nss/lib/ssl/ssl3con.c:12747:18
	    #8 0x5a114ca84635 in ssl3_HandleHandshakeMessage nss/lib/ssl/ssl3con.c:12699:22
	    #9 0x5a114ca8c7d6 in ssl3_HandleHandshake nss/lib/ssl/ssl3con.c:12876:18
	    #10 0x5a114ca8c7d6 in ssl3_HandleNonApplicationData nss/lib/ssl/ssl3con.c:13411:22
	    #11 0x5a114ca8f561 in ssl3_HandleRecord nss/lib/ssl/ssl3con.c:13775:10
	    #12 0x5a114cab5e87 in ssl3_GatherCompleteHandshake nss/lib/ssl/ssl3gthr.c:560:18
	    #13 0x5a114cabce77 in ssl_GatherRecord1stHandshake nss/lib/ssl/sslcon.c:73:10
	    #14 0x5a114c9c66bb in ssl_Do1stHandshake nss/lib/ssl/sslsecur.c:43:14
	    #15 0x5a114c9c971e in SSL_ForceHandshake nss/lib/ssl/sslsecur.c:431:14
	    #16 0x5a114c9b5547 in TlsCommon::DoHandshake(PRFileDesc*, bool) nss/fuzz/targets/lib/tls/common.cc:57:10
	    #17 0x5a114c9b12ef in LLVMFuzzerTestOneInput nss/fuzz/targets/tls_client.cc:58:3
	    #18 0x5a114d11a7a0 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
	    #19 0x5a114d119fc5 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:516:7
	    #20 0x5a114d11bf52 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile>>&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:829:7
	    #21 0x5a114d11c242 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile>>&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:867:3
	    #22 0x5a114d10b37b in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:914:6
	    #23 0x5a114d136752 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
	    #24 0x7c7459188082 in __libc_start_main /build/glibc-LcI20x/glibc-2.31/csu/libc-start.c:308:16
	
	================================================================================
	The following leaks are not necessarily related to the first leak.
	
	
	Direct leak of 1064 byte(s) in 1 object(s) allocated from:
	    #0 0x5a114c971d6c in __interceptor_realloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:82:3
	    #1 0x5a114cadafd9 in PORT_Realloc_Util nss/lib/util/secport.c:101:14
	    #2 0x5a114cabee7c in sslBuffer_Grow nss/lib/ssl/sslencode.c:50:39
	    #3 0x5a114cabf6f2 in sslBuffer_AppendVariable nss/lib/ssl/sslencode.c:117:9
	    #4 0x5a114ca2ef18 in tls13_MaybeGreaseEch nss/lib/ssl/tls13ech.c:2258:10
	    #5 0x5a114ca6cb5a in ssl3_SendClientHello nss/lib/ssl/ssl3con.c:5620:18
	    #6 0x5a114cabd87a in ssl_BeginClientHandshake nss/lib/ssl/sslcon.c:189:10
	    #7 0x5a114c9c66bb in ssl_Do1stHandshake nss/lib/ssl/sslsecur.c:43:14
	    #8 0x5a114c9c971e in SSL_ForceHandshake nss/lib/ssl/sslsecur.c:431:14
	    #9 0x5a114c9b5547 in TlsCommon::DoHandshake(PRFileDesc*, bool) nss/fuzz/targets/lib/tls/common.cc:57:10
	    #10 0x5a114c9b12ef in LLVMFuzzerTestOneInput nss/fuzz/targets/tls_client.cc:58:3
	    #11 0x5a114d11a7a0 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
	    #12 0x5a114d119fc5 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:516:7
	    #13 0x5a114d11bf52 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile>>&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:829:7
	    #14 0x5a114d11c242 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile>>&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:867:3
	    #15 0x5a114d10b37b in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:914:6
	    #16 0x5a114d136752 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
	    #17 0x7c7459188082 in __libc_start_main /build/glibc-LcI20x/glibc-2.31/csu/libc-start.c:308:16
	
	SUMMARY: AddressSanitizer: 2128 byte(s) leaked in 2 allocation(s).

BugBot [:suhaib / :marco/ :calixte]

Comment 1

•

6 months ago

The severity field is not set for this bug.
:beurdouche, could you have a look please?

For more information, please visit BugBot documentation.

Flags: needinfo?(bbeurdouche)

Benjamin Beurdouche [:beurdouche]

Updated

•

6 months ago

Group: crypto-core-security

Flags: needinfo?(bbeurdouche) → needinfo?(djackson)

Daniel Veditz [:dveditz]

Comment 2

•

5 months ago

What is the concern about having a memory leak bug be public?

Keywords: sec-other

Anna Weine

Comment 3

•

5 months ago

(In reply to Maurice Dauer [:mdauer] from comment #0)

Created attachment 9441321 [details]
clusterfuzz-testcase-tls-client-5766569436708864

OSS-Fuzz: https://oss-fuzz.com/testcase-detail/5766569436708864

Details

My uneducated guess would be that we reach this line twice, overwriting ss->ssl3.hs.greaseEchBuf, without having freed the previously allocated greaseBuf.

Reproduction

Download the attached testcase
Build NSS with ./build.sh -c --fuzz=tls
Run /path/to/dist/Debug/bin/nssfuzz-tls-client /path/to/testcase

Stack trace

Direct leak of 1064 byte(s) in 1 object(s) allocated from:
	    #0 0x5a114c971d6c in __interceptor_realloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:82:3
	    #1 0x5a114cadafd9 in PORT_Realloc_Util nss/lib/util/secport.c:101:14
	    #2 0x5a114cabee7c in sslBuffer_Grow nss/lib/ssl/sslencode.c:50:39
	    #3 0x5a114cabf6f2 in sslBuffer_AppendVariable nss/lib/ssl/sslencode.c:117:9
	    #4 0x5a114ca2ef18 in tls13_MaybeGreaseEch nss/lib/ssl/tls13ech.c:2258:10
	    #5 0x5a114ca6cb5a in ssl3_SendClientHello nss/lib/ssl/ssl3con.c:5620:18
	    #6 0x5a114ca9b833 in ssl3_HandleHelloRequest nss/lib/ssl/ssl3con.c:5787:10
	    #7 0x5a114ca84635 in ssl3_HandlePostHelloHandshakeMessage nss/lib/ssl/ssl3con.c:12747:18
	    #8 0x5a114ca84635 in ssl3_HandleHandshakeMessage nss/lib/ssl/ssl3con.c:12699:22
	    #9 0x5a114ca8c7d6 in ssl3_HandleHandshake nss/lib/ssl/ssl3con.c:12876:18
	    #10 0x5a114ca8c7d6 in ssl3_HandleNonApplicationData nss/lib/ssl/ssl3con.c:13411:22
	    #11 0x5a114ca8f561 in ssl3_HandleRecord nss/lib/ssl/ssl3con.c:13775:10
	    #12 0x5a114cab5e87 in ssl3_GatherCompleteHandshake nss/lib/ssl/ssl3gthr.c:560:18
	    #13 0x5a114cabce77 in ssl_GatherRecord1stHandshake nss/lib/ssl/sslcon.c:73:10
	    #14 0x5a114c9c66bb in ssl_Do1stHandshake nss/lib/ssl/sslsecur.c:43:14
	    #15 0x5a114c9c971e in SSL_ForceHandshake nss/lib/ssl/sslsecur.c:431:14
	    #16 0x5a114c9b5547 in TlsCommon::DoHandshake(PRFileDesc*, bool) nss/fuzz/targets/lib/tls/common.cc:57:10
	    #17 0x5a114c9b12ef in LLVMFuzzerTestOneInput nss/fuzz/targets/tls_client.cc:58:3
	    #18 0x5a114d11a7a0 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
	    #19 0x5a114d119fc5 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:516:7
	    #20 0x5a114d11bf52 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile>>&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:829:7
	    #21 0x5a114d11c242 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile>>&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:867:3
	    #22 0x5a114d10b37b in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:914:6
	    #23 0x5a114d136752 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
	    #24 0x7c7459188082 in __libc_start_main /build/glibc-LcI20x/glibc-2.31/csu/libc-start.c:308:16
	
	================================================================================
	The following leaks are not necessarily related to the first leak.
	
	
	Direct leak of 1064 byte(s) in 1 object(s) allocated from:
	    #0 0x5a114c971d6c in __interceptor_realloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:82:3
	    #1 0x5a114cadafd9 in PORT_Realloc_Util nss/lib/util/secport.c:101:14
	    #2 0x5a114cabee7c in sslBuffer_Grow nss/lib/ssl/sslencode.c:50:39
	    #3 0x5a114cabf6f2 in sslBuffer_AppendVariable nss/lib/ssl/sslencode.c:117:9
	    #4 0x5a114ca2ef18 in tls13_MaybeGreaseEch nss/lib/ssl/tls13ech.c:2258:10
	    #5 0x5a114ca6cb5a in ssl3_SendClientHello nss/lib/ssl/ssl3con.c:5620:18
	    #6 0x5a114cabd87a in ssl_BeginClientHandshake nss/lib/ssl/sslcon.c:189:10
	    #7 0x5a114c9c66bb in ssl_Do1stHandshake nss/lib/ssl/sslsecur.c:43:14
	    #8 0x5a114c9c971e in SSL_ForceHandshake nss/lib/ssl/sslsecur.c:431:14
	    #9 0x5a114c9b5547 in TlsCommon::DoHandshake(PRFileDesc*, bool) nss/fuzz/targets/lib/tls/common.cc:57:10
	    #10 0x5a114c9b12ef in LLVMFuzzerTestOneInput nss/fuzz/targets/tls_client.cc:58:3
	    #11 0x5a114d11a7a0 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
	    #12 0x5a114d119fc5 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:516:7
	    #13 0x5a114d11bf52 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile>>&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:829:7
	    #14 0x5a114d11c242 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile>>&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:867:3
	    #15 0x5a114d10b37b in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:914:6
	    #16 0x5a114d136752 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
	    #17 0x7c7459188082 in __libc_start_main /build/glibc-LcI20x/glibc-2.31/csu/libc-start.c:308:16
	
	SUMMARY: AddressSanitizer: 2128 byte(s) leaked in 2 allocation(s).

I think you're right. I think what happens here is the following:

We send a clientHello and call tls13_MaybeGreaseEch
We send the second clientHello, but now with client_hello_renegotiation CH type
Pretty much we arrive to the same function, and again ss->ssl3.hs.greaseEchBuf = greaseBuf;

The thing is that IIUC, client_hello_renegotiation is for <TLS1.3, whereas ECH/Grease are TLS1.3
I would suggest to extend tls13_MaybeGreaseEch with CH type and return SECSuccess if we see client_hello_renegotiation.
And to put PORT_Assert(ss->ssl3.hs.greaseEchBuf.len == 0);

Important, I don't know ECH enough and my suggestion could be incorrect!

Comment hidden (obsolete)

~~I'm looking at this again and noticed that in tls13_MaybeGreaseEch there is a check for TLS1.3:~~

    if (ss->vrange.max < SSL_LIBRARY_VERSION_TLS_1_3 ||
        IS_DTLS(ss)) {
        return SECSuccess;
    }

~~but AFAIU ss->vrange.max is only the highest possible protocol version allowed by the user (and not necessarily the one used in the connection), so shouldn't the check instead be:~~

    if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3 ||
        IS_DTLS(ss)) {
        return SECSuccess;
    }

Maurice Dauer [:mdauer]

Reporter

Comment 5

•

3 months ago

Attached file WIP: Bug 1934877 - Fix memory leak in tls13_MaybeGreaseEch (obsolete) — Details

Maurice Dauer [:mdauer]

Reporter

Updated

•

3 months ago

Assignee: nobody → mdauer

Status: NEW → ASSIGNED

Phabricator Automation

Updated

•

3 months ago

Attachment #9471094 - Attachment description: Bug 1934877 - Correct TLS version check for ECH/Grease, r?#nss-reviewers → WIP: Bug 1934877 - Fix memory leak in tls13_MaybeGreaseEch

Dennis Jackson

Assignee

Updated

•

2 months ago

Blocks: nss-fuzzing-bugs

Phabricator Automation

Updated

•

1 month ago

Attachment #9471094 - Attachment is obsolete: true

Maurice Dauer [:mdauer]

Reporter

Updated

•

1 month ago

Assignee: mdauer → nobody

Status: ASSIGNED → NEW

Dennis Jackson

Assignee

Updated

•

1 month ago

Assignee: nobody → djackson

Flags: needinfo?(djackson)

Dennis Jackson

Assignee

Comment 6

•

1 month ago

Attached file Bug 1934877 - Renegotiations should use a fresh ECH GREASE buffer. r=#nss-reviewers — Details

Dennis Jackson

Assignee

Comment 7

•

1 month ago

You were both spot on with the diagnosis, the test harness just needed a small tweak to force the server to use 1.2 and trigger renegotiation correctly.

This is a really nice find insofar as it is absolutely a scenario we (I) could of and should of written a test for, but it slipped through.

Dennis Jackson

Assignee

Comment 8

•

1 month ago

https://hg.mozilla.org/projects/nss/rev/fbc037732c7ef97c00bf6a22eb311d6e2a1d2040

Status: NEW → RESOLVED

Closed: 1 month ago

Resolution: --- → FIXED

Dennis Jackson

Assignee

Updated

•

1 month ago

Group: crypto-core-security

Keywords: sec-other

clusterfuzz-testcase-tls-client-5766569436708864 6 months ago Maurice Dauer [:mdauer] 1.50 KB, application/octet-stream		Details
WIP: Bug 1934877 - Fix memory leak in tls13_MaybeGreaseEch 3 months ago Maurice Dauer [:mdauer] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1934877 - Renegotiations should use a fresh ECH GREASE buffer. r=#nss-reviewers 1 month ago Dennis Jackson 48 bytes, text/x-phabricator-request		Details \| Review